-
Notifications
You must be signed in to change notification settings - Fork 68
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
contrib: add key/cert options and signature check for sbsign #695
base: master
Are you sure you want to change the base?
Conversation
} else { | ||
die "Sign method $SignMethod not valid."; | ||
} | ||
|
||
if ( $DeleteUnsigned && $SignMethod eq "sbctl" ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this whole block removed? It seems like it will fundamentally change the behavior of this script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really? It does only 1 of the 2 things (when DeleteUnsigned
is enabled):
- Print a message if
sbctl
is used - Clean up after the inability of
sbsign
to sign in place... with the new code, that is no longer the case
Maybe I should remove the option DeleteUnsigned
since it is no longer needed.
What am I missing? I see this block of code as no longer needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure that sbsign won't corrupt its input if you try to overwrite it with output? Is that the case for several prior versions of sbsign?
If it's safe, the better approach would be to replace DeleteUnsigned
with something a bit more meaningful like Overwrite
and handle that above by just changing what you're passing as the output name to sbsign.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I propose UseOriginalFileName
:
- true -> *.efi = stays the same.
- false (default) -> *.signed.efi
Or you can think of a better boolean var name?
And I will write the logics to always output to a different file. (solving above discussion)
Do you still want to offer the option, that is available for the sbsigntools
method, to keep the original unsigned EFI files?
Since sbctl
signs in place, why would we want to keep the unsigned EFI files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I proposed, and still prefer, Overwrite
, which is both shorter and more descriptive.
The commit message will also need to be formulated to be consistent with previous commits to this project. |
I did my best with the PR title. Feel free to edit it to match the project standards. I have tested it on Debian Bookworm with sbsigntool version 0.9.4-3.1. |
@@ -56,9 +60,9 @@ | |||
or die "Cannot open ZBM dir: $ZBM"; | |||
|
|||
if ($SignBackups) { | |||
@EFIBins = grep { !/signed\.efi$/i and /\.efi/i } readdir $ZBM_dir; | |||
@EFIBins = sort grep { !/signed\.efi$/i and /\.efi/i } readdir $ZBM_dir; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why sort?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Glad you asked. The sbverify
command modifies the efi files last modified timestamp resulting in refind showing as the default ZBM the last of the this array (last of the array is first in rEFInd list). This sort
solution allows the user to controll the rEFInd order via naming.
Do you see an issue with using sort?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is not the place to hack around rEFInd sorting specifics. There is no need to sort unless this script depends on the order of the array, which it does not.
} else { | ||
@EFIBins = grep { !/signed\.efi$/i and !/backup/i and /\.efi/i } readdir $ZBM_dir; | ||
@EFIBins = sort grep { !/signed\.efi$/i and !/backup/i and /\.efi/i } readdir $ZBM_dir; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, why sort?
contrib/zbm-sign.pl
Outdated
@@ -72,17 +76,14 @@ | |||
if ( $SignMethod eq "sbctl" ) { | |||
system "sbctl sign $ZBM/$_"; | |||
} elsif ( $SignMethod eq "sbsign" ) { | |||
$Unsigned = substr( $_, 0, -4 ); | |||
system "sbsign --key $KeyDir/DB.key --cert $KeyDir/DB.crt $ZBM/$_ --output $ZBM/$Unsigned-signed.efi"; | |||
my $verify_output = "sbverify --cert $KeyDir/$CrtFileName $ZBM/$_ 2>&1"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You aren't actually running this command, so the test on the output you are expecting will always fail.
Furthermore, if possible, it would be better to distinguish whether the input is already signed by the exit code, rather than grepping output for a magic string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Off-topic: I'm glad I'm invested time into this PR.
On-topic: This exact line of code behaves as expected. I can see it only signs the files that need signing. How do you explain it?
Should I replace the double qoutes with qx or backticks? What do you prefer?
I will look into the exit codes and see if that is an option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is not with the sbsign
call.
my $verify_output = "sbverify --cert $KeyDir/$CrtFileName $ZBM/$_ 2>&1";
just sets a string, it doesn't actually run sbverify
.
contrib/zbm-sign.pl
Outdated
say "File $_ is already signed."; | ||
next; | ||
} | ||
system "sbsign --key $KeyDir/$KeyFileName --cert $KeyDir/$CrtFileName $ZBM/$_ --output $ZBM/$_"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replacing the unsigned input with the signed output is destructive and probably shouldn't be the default behavior. It seems better to leave this as it was, creating a separate signed file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is exactly the default behavior of sbctl
. The Arch wiki shows an example using the same approach:
sbsign --key db.key --cert db.crt --output /boot/vmlinuz-linux /boot/vmlinuz-linux
Have you had bad experiences with sbsign
leaving you with a corrupt file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never assume that a process will complete successfully, especially when you are changing existing behavior that is less destructive. As I noted earlier, replacing the DeleteUnsigned
option with an Overwrite
and using that to control whether --output
is passed at all is a better approach than always clobbering inputs.
} else { | ||
die "Sign method $SignMethod not valid."; | ||
} | ||
|
||
if ( $DeleteUnsigned && $SignMethod eq "sbctl" ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure that sbsign won't corrupt its input if you try to overwrite it with output? Is that the case for several prior versions of sbsign?
If it's safe, the better approach would be to replace DeleteUnsigned
with something a bit more meaningful like Overwrite
and handle that above by just changing what you're passing as the output name to sbsign.
my $KeyFileName = $SecureBoot->{KeyFileName}; | ||
my $CrtFileName = $SecureBoot->{CrtFileName}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Name
part seems superfluous, and CertFile
would be a bit more readable than CrtFile
.
Closes #693